client: bitcoin spv #1230

buck54321 · 2021-10-11T23:28:39Z

Add a built-in Bitcoin SPV wallet. Uses neutrino and btcwallet.

client/asset:

Add Create and Exists methods to the Driver. Create is used to
create a "seeded" (native, built-in) wallet, and should
not be used for non-seeded wallets. Exists checks the existence of
a wallet. Exists is written so far to check even RPC wallet
existence, though it's only used for seeded wallets right now.

Adds the WalletDefinition type. WalletInfo has a new field,
AvailableWallets []*WalletDefinition.

WalletConfig type has a new field, Type string, which should match
the Type field of one of the AvailableWallets. Existing backends
must handle Type == "" for pre-existing wallets.

client/asset/btc:

Quite a few changes to configuration parsing to more clearly
separate RPC config from wallet settings. New spvWallet type
manages the *btcwallet.Wallet and *neutrino.ChainService. The
Wallet interface was already defined, so the work was just
implementing the methods using direct calls to wallet methods.

Everything is run through an interface for testing. Existing tests
required relatively few modifications to work with spv as well.
There are just a couple of new unit tests specific to the more
nuanced spv methods. The regnet_test.go/livetest.go test is adapted
to run an all-SPV trio for one test.

client/core: Work with new wallet definitions and give special handling
to seeded wallets.

front-end:

NewWalletForm and WalletConfigForm updated to work with
wallet definitions. Wallet type is selected via a tab at the top of
the NewWalletForm. Seeded wallets take no user-supplied password
(enforced in core). Special handling of headers and options so that
creating a seeded wallet can be very close to a 1-button, 1-click
experience.

A new form is added after the new wallet form conditionally if the
wallet balance is lower than required for the fee. This will always
be the case for the first native spv wallet. The new form shows a
deposit address and the current balance, and directs the user to
fund the address for registration fees. The balance is checked in
a loop and progresses back to the fee confirmation form when the
balance is sufficient. When working on simnet, just fund it from
the alpha node in harness-ctl.

client/db:

Store the new Type field as part of the wallet. I wrote
an upgrade function to store an empty string for the new fields
position in the database, but we're not populating it anyway,
and existing wallets handle that, so I don't see a good reason
to "upgrade".

buck54321 · 2021-10-12T12:48:31Z

I still have concerns with potentially performing long unnecessary scans. Let's say the taker's swap is sitting in mempool due to fee volatility. Every 5 seconds (the interval of the TickerQueue), the maker will call AuditContract, which is going to calculate a start block based on match time (not our tx), which will result in ~ 20 blocks too many, and then will scan from there to the tip. The scan will go every 5 seconds until the match is found or revoked.

We could avoid these scans if we had some kind of checkpoint system. A way to say "we already looked for this output, and didn't see it through block X, so restart the scan there (if not orphaned)". Sorta like what we do with FindRedemption.

Update: solution in bbd1669

client/asset/btc/spv.go

martonp · 2021-10-12T21:14:24Z

client/asset/driver.go

+type CreateWalletParams struct {
+	Type     string
+	Seed     []byte
+	Pass     []byte


Shouldn't the password be a string since a string is passed to wallet.Unlock?

Hmm. Good point. I'm tempted to go the other way though, and change the (Wallet).Unlock method to accept a []byte.

Won't that require a bunch of changes for non seeded wallets? The geth node takes a string to lock/unlock the private key as well.

It will require changing existing implementations, but it's not a big job. The thing about string passwords is that once we're done with them, we're at the mercy of the garbage collector to get rid of the sensitive data. But we can zero a []byte. We even have a type for it, encode.PassBytes. It's true that eth will still be converting to string. So will dcr and btc-rpc, but we can now avoid strings for btc-spv, and we should take the opportunity to make things more secure.

OK, makes sense.

I thought we had a dex.Bytes type for passwords.

chappjc

Partial review. Looks really solid so far. No major issues.

go.mod

client/asset/btc/spv.go

chappjc · 2021-10-12T18:40:45Z

client/asset/btc/spv.go

+// 3. We don't see a mempool. We're blind to new transactions until they are
+//    mined. This requires special handling by the caller. We've been
+//    anticipating this, so Core and Swapper are permissive of missing acks for
+//    audit requests.


although Core responds to the audit request based on the txData, right? This is a note to self to check

We're actually not yet, but I think that's where we're headed.

https://github.com/buck54321/dcrdex/blob/fb0039141da1d0ff47c7254f32b9a0bde055f79f/client/asset/btc/btc.go#L1838-L1839

client/asset/btc/spv.go

chappjc

Just a few more comments in client/asset/btc before I move to core and webserver

chappjc · 2021-10-13T15:47:27Z

client/asset/btc/spv.go

+		if len(msgTx.TxOut) < int(vout+1) {
+			return nil, 0, fmt.Errorf("wallet transaction %s found, but not enough outputs for vout %d", txHash, vout)
+		}
+		return msgTx.TxOut[vout], confs, nil


In this case where walletTransaction found it, it returns a non-nil txout regardless of spent status, which is at odds with the docs for getTxOut. Only for WalletTransactionNotFound where it calls scanFilters does it emulate gettxout returning nil when spent.
Basically, the behavior matches for non-wallet txns, but for wallet txns, it will never appear spent. Are we ok with that?
If it's a wallet output too, could check txDetails.Credits[].Spent? If its not in the Credits slice, then it's a non-wallet-output (right?) like own swap, and then I don't know... just return as if not spent I guess?

chappjc · 2021-10-13T15:55:14Z

client/asset/btc/spv.go

+			w.log.Debugf("A spending input was found during the scan but the output " +
+				"itself wasn't found. Was the startBlockHeight early enough?")


Warnf? What's the severity and resolution if this happens? clear the checkpoint so it starts over from match time?

Warnf seems appropriate. We don't store a *scanCheckpoint in this case.

This place this ends up affecting us is in SwapConfirmation, we return confs = 0 (which is wrong) and spent = true. In situations where we're looking for the counter-party's swap, we revoke if it's found to be spent, without inspecting the confs, so we're all good there. When it's our output, we'll know the block and won't end up here, but even if we did, we just end up sending out some inaccurate Data-severity notifications to the UI until the match progresses.

client/asset/btc/spv.go

client/asset/btc/btc.go

chappjc · 2021-10-13T19:11:31Z

Got an intermittent AuditContract error in livetest.

EDIT: Appears resolved with latest commits.

////////// SPV WALLET W/O SPLIT //////////
2021-10-13 14:10:17.131 [INF] TEST: btc:alpha- has reported a new block, error = <nil>
2021-10-13 14:10:19.438 [INF] TEST[alpha]: alpha with address bcrt1qt3585tn3sfes6mdh955pzu2ws4ul2rcjzrfw4c is synced with balance = &{Available:8400000000 Immature:0 Locked:0} 

2021-10-13 14:10:23.563 [INF] TEST: btc:beta- has reported a new block, error = <nil>
2021-10-13 14:10:25.830 [INF] TEST[beta]: beta with address bcrt1qvz6jpkkz9tqdrzvpta5anw9y5x2ak47ahhtsjk is synced with balance = &{Available:8400000000 Immature:0 Locked:0} 

2021-10-13 14:10:29.483 [INF] TEST: btc:beta- has reported a new block, error = <nil>
2021-10-13 14:10:29.959 [INF] TEST: btc:alpha-gamma has reported a new block, error = <nil>
2021-10-13 14:10:32.281 [INF] TEST[gamma]: gamma with address bcrt1qsj2tsn6w3wzel8ygj63len3msctmnag77u69ys is synced with balance = &{Available:8400000000 Immature:0 Locked:0} 

2021-10-13 14:10:33.063 [INF] TEST: btc:alpha- has reported a new block, error = <nil>
2021-10-13 14:10:34.813 [INF] TEST: Wallets configured
2021-10-13 14:10:34.813 [INF] TEST: alpha 84.000000 available, 0.000000 immature, 0.000000 locked
2021-10-13 14:10:34.813 [INF] TEST: beta 84.000000 available, 0.000000 immature, 0.000000 locked
2021-10-13 14:10:34.813 [INF] TEST: gamma 84.000000 available, 0.000000 immature, 0.000000 locked
2021-10-13 14:10:34.813 [INF] TEST: Testing FundOrder
2021-10-13 14:10:34.814 [INF] TEST[gamma]: Funding 0.06 btc order with coins [638fe21ae4b031b43f74e12ad9d1361b65af41cfc38ebe7f5ae557fa9c5e6d06:0] worth 1
2021-10-13 14:10:34.814 [INF] TEST[gamma]: Funding 0.06 btc order with coins [8b6a5f9d4b7147844d3f559c8cb7e026f88a9813381b1ea1c0a3de1a9b25153a:0] worth 3
2021-10-13 14:10:34.814 [INF] TEST[gamma]: Funding 0.06 btc order with coins [638fe21ae4b031b43f74e12ad9d1361b65af41cfc38ebe7f5ae557fa9c5e6d06:0] worth 1
2021-10-13 14:10:34.814 [INF] TEST[gamma]: Funding 0.02 btc order with coins [638fe21ae4b031b43f74e12ad9d1361b65af41cfc38ebe7f5ae557fa9c5e6d06:0] worth 1
2021-10-13 14:10:34.814 [INF] TEST[gamma]: Funding 0.04 btc order with coins [8b6a5f9d4b7147844d3f559c8cb7e026f88a9813381b1ea1c0a3de1a9b25153a:0] worth 3
2021-10-13 14:10:34.814 [INF] TEST: Testing Swap
2021-10-13 14:10:34.815 [WRN] TEST[gamma]: Unable to get optimal fee rate, using fallback of 100: EstimateSmartFee not available on spv
2021-10-13 14:10:34.815 [WRN] TEST[gamma]: Unable to get optimal fee rate, using fallback of 100: EstimateSmartFee not available on spv
2021-10-13 14:10:39.816 [INF] TEST: Sent 2 swaps
2021-10-13 14:10:39.816 [INF] TEST:       Swap # 1: 9a4a5f686ab7e8e5b13593353636ac5cb84dd7cc97dd558c90754b0fe59f45e3:0
2021-10-13 14:10:39.816 [INF] TEST:       Swap # 2: 9a4a5f686ab7e8e5b13593353636ac5cb84dd7cc97dd558c90754b0fe59f45e3:1
2021-10-13 14:10:39.979 [INF] TEST: btc:alpha-gamma has reported a new block, error = <nil>
2021-10-13 14:10:40.483 [INF] TEST: btc:beta- has reported a new block, error = <nil>
2021-10-13 14:10:42.821 [INF] TEST: Testing AuditContract
    livetest.go:359: error auditing contract: error finding unspent contract: 9a4a5f686ab7e8e5b13593353636ac5cb84dd7cc97dd558c90754b0fe59f45e3:0 : output 9a4a5f686ab7e8e5b13593353636ac5cb84dd7cc97dd558c90754b0fe59f45e3:0 not found
--- FAIL: TestWallet (31.90s)

chappjc · 2021-10-13T19:45:17Z

Here's a weird one that's not related to logging at all, all inside neutrino and btcd code..

EDIT: looks to be a flaw with the btcd AddrManager's API design. See here.

==================
WARNING: DATA RACE
Write at 0x00c0004251a0 by goroutine 54:
  github.com/btcsuite/btcd/addrmgr.(*AddrManager).updateAddress()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/addrmgr/addrmanager.go:189 +0x3d5
  github.com/btcsuite/btcd/addrmgr.(*AddrManager).AddAddresses()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/addrmgr/addrmanager.go:605 +0xfe
  github.com/lightninglabs/neutrino.(*ServerPeer).OnAddr()
      /home/jon/go/pkg/mod/github.com/lightninglabs/[email protected]/neutrino.go:378 +0x6f2
  github.com/lightninglabs/neutrino.(*ServerPeer).OnAddr-fm()
      /home/jon/go/pkg/mod/github.com/lightninglabs/[email protected]/neutrino.go:318 +0x4d
  github.com/btcsuite/btcd/peer.(*Peer).inHandler()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/peer/peer.go:1399 +0x1403
  github.com/btcsuite/btcd/peer.(*Peer).start·dwrap·5()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/peer/peer.go:2158 +0x39

Previous read at 0x00c0004251a0 by goroutine 242:
  github.com/btcsuite/btcd/addrmgr.(*KnownAddress).NetAddress()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/addrmgr/knownaddress.go:28 +0x1da
  github.com/lightninglabs/neutrino.NewChainService.func3()
      /home/jon/go/pkg/mod/github.com/lightninglabs/[email protected]/neutrino.go:773 +0x1e6
  github.com/btcsuite/btcd/connmgr.(*ConnManager).NewConnReq()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/connmgr/connmanager.go:391 +0x347
  github.com/btcsuite/btcd/connmgr.(*ConnManager).Start·dwrap·7()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/connmgr/connmanager.go:530 +0x39

Goroutine 54 (running) created at:
  github.com/btcsuite/btcd/peer.(*Peer).start()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/peer/peer.go:2158 +0x477
  github.com/btcsuite/btcd/peer.(*Peer).AssociateConnection.func1()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/peer/peer.go:2193 +0x34

Goroutine 242 (running) created at:
  github.com/btcsuite/btcd/connmgr.(*ConnManager).Start()
      /home/jon/go/pkg/mod/github.com/btcsuite/[email protected]/connmgr/connmanager.go:530 +0x1ed
  github.com/lightninglabs/neutrino.(*ChainService).Start·dwrap·21()
      /home/jon/go/pkg/mod/github.com/lightninglabs/[email protected]/neutrino.go:1541 +0x39
==================

chappjc

That's it for my first pass. Just about everything looks fine.
Clearly there's something funny with the intermittent audit failures in livetest, and a whole lot of data races with neutrino and/or logging, but conceptually I don't see anything really wrong.

client/asset/btc/livetest/regnet_test.go

client/asset/dcr/dcr.go

chappjc · 2021-10-13T20:23:16Z

client/asset/dcr/dcr.go

+	if coinNotFound {
+		return nil, asset.CoinNotFoundError
+	}


Ok, so more actual auditing of the txData, but not totally passing just yet if it's not found on the network...
And if we decided to just pass the audit from txData and ack to server, we could just remove this check?
Any reason to delay on that change, just one step at a time? I think @itswisdomagain was proposing to simply audit a second time when the counterparty contract is confirmed, before broadcasting anything of our own.

I think maybe we should just audit (and re-broadcast?) the tx data in AuditContract without finding the output on-chain. We'll ack based on that, and then worry about finding the transaction with SwapConfirmation and be more permissive of CoinNotFound error there. I don't personally see a lot of value in double checking that the on-chain contract validates the same as the txData, but it looks like @itswisdomagain thought it was a good idea. What do you think @itswisdomagain? Can we get rid of a coin search in AuditContract altogether.

and be more permissive of CoinNotFound error there

I guess this is the twist.

I don't personally see a lot of value in double checking that the on-chain contract validates the same as the txData

Felt prudent to me too, but I'm not totally set on that.

I had some concerns about the possibility of finding an on-chain contract that differs from the txData that was validated, differs enough to have failed audit checks but after further consideration, the possibility became less real to me. Still felt safer to repeat audit before broadcasting a counter-swap or redeeming, but I'm not set on that either.

Wrote my initial thoughts on this topic here: #788 (comment).

client/webserver/site/src/css/market_dark.scss

client/webserver/site/src/html/register.tmpl

client/webserver/site/src/js/app.js

dex/networks/btc/config.go

go.mod

chappjc · 2021-10-14T00:42:54Z

Here's a weird one that's not related to logging at all, all inside neutrino and btcd code..

This does look like a btcd API design issue that makes the btcd/addrmgr.(*AddrManager).GetAddress method essentially unusable by consumers. Considering a PR: https://github.com/btcsuite/btcd/compare/master...chappjc:safe-addr-accessors?expand=1
Simply changing GetAddress to return a copy works too, but breaks their unit tests, and apparently the intended semantics of the API, even if it is impossible to use without a data race except in a contrived scenario like in the addrmgr unit tests.

neutrino has a virtually identical newAddressFunc closure that triggers this unavoidable data race, which I observed in my previous comment: https://github.com/lightninglabs/neutrino/blob/51686db787e01a3709b1583af3e20e916811d585/neutrino.go#L767-L773

If anyone else would care to look at it and give feedback I'd appreciate it. I'm not decided yet if I should raise the issue with the neutrino and/or btc devs. I've pinged @davecgh for his eyes on this.

JoeGruffins

Some inline comments.

client/asset/bch/bch.go

client/asset/btc/btc.go

JoeGruffins · 2021-10-13T09:44:59Z

client/asset/btc/btc.go

+
+	if coinNotFound {
+		return nil, asset.CoinNotFoundError
+	}


This seems wrong. This doesn't change since the last check and there's no positive/non-err return path between there and here. It could return much earlier.

client/asset/btc/spv.go

chappjc · 2021-10-14T20:34:12Z

Different race, but also related to logging. I see a bunch that are with livetest.go:63 and livetest.go:69 like this.

Seems like it's happening because btc.NewWallet is called in createWallet in regnet_test.go and again in spvConstructor just below it. Unless I'm mistaken, we want createWallet to return the wallet to spvConstructor

EDIT: Hmm, now I see you are doing a cm.Disconnect in createWallet, so that it is a temporary wallet instance. However, It appears that there's no synchronization in Disconnect to make sure the w.stop() is actually executed before unblocking.

EDIT 2: confirmed that at least some of the test races are because wallet shutdown wasn't waiting on stop. The annoying global neutrino/btcsuite loggers were also a deal breaker since you cannot set them up again with a running spvWallet, like we have with three of them in regnet_test.go. Messy hacking to illustrate where I arrived: 0b0759d

itswisdomagain

Haven't really gone through all the changes yet, was just going through the changes to the asset.Driver interface and have a question. Will do a more thorough review later.

itswisdomagain · 2021-10-15T02:37:20Z

client/core/core.go

+			return newError(passwordErr, "cannot set a password on a seeded wallet")
+		}
+
+		exists, err := asset.WalletExists(assetID, form.Type, c.assetDataDirectory(assetID), form.Config, c.net)


The btc driver implementation of this method (haven't checked others) expects that this method may be called for RPC wallets as well. Is that the case or is that planned?

Yeah, but I think I'm going to remove this RPC checking capability and just doc that the method is for seeded wallets only. Does that work?

Fine with me.

Although the seeded vs built-in terminology can be confusing I think because as has been pointed out, most wallets are seeded even the external ones, when really when we say "seeded" we mean created and managed by the dex process. Internally we have solidified the seeded concept though, even for wallets like eth where we're shoehorning the seed because geth has no idea what a seed is for.

Whatever makes sense to you though. These are code docs, not user-facing messages.

Internally we have solidified the seeded concept though

I generally prefer to use descriptive terms rather than potentially confusing terms which meanings may need to be explained to new/other devs. Yes, the seeded concept has been clarified but docs and variable names should also reflect this clarification so the confusion don't re-arise tomorrow.

martonp · 2021-10-15T19:40:21Z

client/asset/interface.go

@@ -22,6 +22,27 @@ const (
 	ErrUnsupported    = dex.ErrorKind("unsupported")
 )

+type WalletDefinition struct {
+	// If seeded is true, the Setup method will be provided a detereministic


Create method instead of Setup method.

client/asset/driver.go

client/asset/btc/btc.go

client/db/types.go

JoeGruffins · 2021-10-18T02:19:17Z

client/db/bolt/upgrades.go

+// Probably not worth doing. Just let decodeWallet_v0 append the nil and pass it
+// up the chain.


I think you should still increment a database version. If one had a wallet update, silently, and tried to use the previous version with the same database, they might not know why it no longer works.

Trying it out, that is going back to master from this pr, there is this log, that doesn't stop anything, but I think upping the db version would and should.

[ERR] CORE: error loading wallets from database: DecodeWallet error: unknown DecodeWallet version 1

And then there is no indication on the UI, it spins for a while, and you can add wallets again, but if you reset the app they don't load again.

itswisdomagain

First pass. A few wording change suggestions but as previously discussed, no need to waste time on them right away if they aren't straightforward.

Next, I'll look at the btc spv changes in particular and the btc AuditContract and SwapConfirmations changes.

itswisdomagain · 2021-10-16T23:25:25Z

client/asset/driver.go

 )

 var (
 	driversMtx sync.RWMutex
 	drivers    = make(map[uint32]Driver)
 )

+// CreateWalletParams are the parameters for internal wallet creation. The


Sorry to bring this up and doesn't need to be addressed right away or at all even, but this sounds like these parameters are only meant for builtin/internal wallets. If dev wordings in other places are selected to accommodate the possibility of using CreateWallet with external wallets, might as well do so here.

itswisdomagain · 2021-10-18T08:49:38Z

client/asset/driver.go

+// CreateWalletParams are the parameters for internal wallet creation. The
+// Settings provided should be the same wallet configuration settings passed to
+// Create.


I asked about this in #1225, I do not completely understand these references to Settings. Is my understanding below correct?

The settings provided - the Settings field of this CreateWalletParams
wallet configuration settings passed to ... - the settings passed to OpenWallet?

I'm assuming this is saying it's the same settings map in WalletConfig, passed to OpenWallet, as you say.

Yes. Same typo here as there. Sorry about that.

itswisdomagain · 2021-10-18T08:54:30Z

client/asset/bch/bch.go

+		AvailableWallets: []*asset.WalletDefinition{{
+			Type:              walletTypeRPC,
+			Tab:               "External",
+			Description:       "Connect to bitcoind",


Also uses a bitcoind binary? Maybe a comment if so, quite surprising and confusing. I also noticed above that walletTypeRPC = "bitcoindRPC".

itswisdomagain · 2021-10-18T08:57:29Z

client/asset/bch/bch.go

+	netPorts = dexbtc.NetPorts{
+		Mainnet: "8332",
+		Testnet: "18332",
+		Simnet:  "18443",
+	}


Not a biggie, but doesn't this increase the scope and lifetime of this variable, compared to when it was just local to a function?

client/asset/btc/livetest/livetest.go

client/asset/btc/btc.go

chappjc · 2021-10-18T13:40:07Z

client/asset/btc/btc.go

@@ -2092,8 +2313,8 @@ func (btc *ExchangeWallet) Address() (string, error) {

 // PayFee sends the dex registration fee. Transaction fees are in addition to
 // the registration fee, and the fee rate is taken from the DEX configuration.
-func (btc *ExchangeWallet) PayFee(address string, regFee uint64) (asset.Coin, error) {
-	txHash, vout, sent, err := btc.send(address, regFee, btc.feeRateWithFallback(1, 0), false)
+func (btc *ExchangeWallet) PayFee(address string, regFee, feeRateSuggestion uint64) (asset.Coin, error) {


We still need similar changes for refund redeem and withdraw I think.
Just need to be careful not to make server RPCs to fetch the rate on paths that block other actions.

buck54321 · 2021-10-21T12:17:29Z

Looks like our balance check is a race with the wallet's internal scan after a block notification. It took two blocks to see a balance when it should have been one. Damn.

Add a built-in Bitcoin SPV wallet. Uses neutrino and btcwallet. client/asset: Add Create and Exists methods to the Driver. Create is used to create a "seeded" (native, built-in) wallet, and should not be used for non-seeded wallets. Exists checks the existence of a wallet. Exists is written so far to check even RPC wallet existence, though it's only used for seeded wallets in production. Adds the WalletDefinition type. WalletInfo has a new field, AvailableWallets []*WalletDefinition. WalletConfig type has a new field, Type string, which should match the Type field of one of the AvailableWallets. Existing backends must handle Type == "" for pre-existing wallets. client/asset/{dcr,ltc,bch,eth}: minor adaptations to fit the Driver and WalletDefinition changes. client/asset/btc: Quite a few changes to configuration parsing to more clearly separate RPC config from wallet settings. New spvWallet type manages the *btcwallet.Wallet and *neutrino.ChainService. The Wallet interface was already defined, so the work was just implementing the methods using direct calls to wallet methods. Everything is run through an interface for testing. Existing tests required relatively few modifications to work with spv as well. There are just a couple of new unit tests specific to the more nuanced spv methods. The regnet_test.go/livetest.go test is adapted to run an all-SPV trio for one test. client/core: Work with wallet definition and give special handling to seeded wallets. front-end: NewWalletForm and WalletConfigForm updated to work with wallet definitions. Wallet type is selected via a tab at the top of the NewWalletForm. Seeded wallets take no user-supplied password (enforced in core). Special handling of headers and options so that creating a seeded wallet can be very close to a 1-button, 1-click experience. A new form is added after the new wallet form conditionally if the wallet balance is lower than required for the fee. This will always be the case for the first native spv wallet. The new form shows a deposit address and the current balance, and directs the user to fund the address for registration fees. The balance is checked in a loop and progresses back to the fee confirmation form when the balance is sufficient. When working on simnet, just fund it from the alpha node in harness-ctl. client/db: Store the new Type field as part of the wallet. I wrote an upgrader to add the new field, but we're not populating it anyway, and existing wallets handle that, so I don't see a good reason to "upgrade".

Refactor initialization and connection. Move nearly all instantiation to connect. There is an assumption here that the caller will never try to use an unconnected wallet to do anything. Upon inspection, I think this is how we've designed things, but it's worth mentioning since calling some methods before connecting will definitely panic now. Log btcwallet and neutrino output to a rotating log file. Combine loadChainClient and startWallet functions into a single (*spvWallet).startWallet method.

Unlike bitcoind, btcwallet has option to subtract fees from sent value. Implement send with subtract from scratch.

buck54321 · 2021-10-21T16:41:35Z

We still have a lag in reporting correct balance, but other issues should be addressed now.

chappjc · 2021-10-21T18:37:47Z

client/core/core.go

+			oldDef, err := walletDefinition(assetID, oldWallet.walletType)
+			// Error can be normal if the wallet was created before wallet types
+			// were a thing. Just assume this is an old wallet and therefore not
+			// seeded.
+			if err == nil && oldDef.Seeded {


You already have oldDef from 30 lines up before if walletDef.Seeded.
It also looks like you solved the problem in the comment with LegacyWalletIndex

Should be good without the walletDefinition call and using the oldDef you have like:

if oldDef.Seeded && oldWallet.connected() { oldWallet.Disconnect() restartOnFail = true }

NVM, I have some updates that will follow. Going to merge this shortly.

chappjc

Working well for the most part. We're all going to start iterating on issues we find.

I have a ReconfigureWallet update coming to address setting the password on the seeded wallet.

chappjc self-requested a review October 12, 2021 00:02

buck54321 force-pushed the btcspv branch from 4bebebd to 0bca468 Compare October 12, 2021 12:24

buck54321 force-pushed the btcspv branch from bbd1669 to d5f0540 Compare October 12, 2021 15:10

chappjc reviewed Oct 12, 2021

View reviewed changes

client/asset/btc/spv.go Show resolved Hide resolved

buck54321 force-pushed the btcspv branch from d5f0540 to fb00391 Compare October 12, 2021 15:40

martonp reviewed Oct 12, 2021

View reviewed changes

chappjc requested changes Oct 12, 2021

View reviewed changes

buck54321 force-pushed the btcspv branch from 1086370 to eef4ef0 Compare October 13, 2021 11:28

chappjc reviewed Oct 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

chappjc requested changes Oct 13, 2021

View reviewed changes

JoeGruffins reviewed Oct 14, 2021

View reviewed changes

chappjc mentioned this pull request Oct 14, 2021

client/core,client/asset/eth: Derive account from app seed #1225

Merged

chappjc reviewed Oct 14, 2021

View reviewed changes

client/asset/btc/spv.go Outdated Show resolved Hide resolved

itswisdomagain suggested changes Oct 15, 2021

View reviewed changes

itswisdomagain mentioned this pull request Oct 15, 2021

client/asset/dcr: refactor to support non-rpc wallets #1227

Merged

buck54321 force-pushed the btcspv branch 2 times, most recently from 537182e to 725d561 Compare October 15, 2021 17:29

martonp reviewed Oct 15, 2021

View reviewed changes

buck54321 commented Oct 17, 2021

View reviewed changes

client/asset/driver.go Outdated Show resolved Hide resolved

JoeGruffins reviewed Oct 18, 2021

View reviewed changes

itswisdomagain suggested changes Oct 18, 2021

View reviewed changes

chappjc reviewed Oct 18, 2021

View reviewed changes

buck54321 force-pushed the btcspv branch from 6c32aec to 1d7f5a6 Compare October 18, 2021 22:36

buck54321 and others added 21 commits October 21, 2021 10:58

remove hashmap

4a27537

drop spendingTxs cache. add scan checkpoint cache

7e13921

nest nil blockHash check. use []byte passwords everywhere

284052b

remove dev docs

e13925f

review followup 2.1, send sendToAddress with subtract

c1046e3

Unlike bitcoind, btcwallet has option to subtract fees from sent value. Implement send with subtract from scratch.

review followup 3

2b6ecf8

backend teardown

5084c09

review followup 4

f5c5c38

refactor Driver interfaces

47e512a

localization. fix shutdown bug

856ee5b

require package init

8ac1c89

clarify Seeded field description

644c1e5

checkpoint

af38f4c

most of wisdoms review

97f7a39

chappjc 5

55daa99

better logNeutrino sequence

f147edf

modify trade_simnet_test to use btc spv wallet

5c71c93

more joegruffins review

350557b

reconfig bug. CoinNotFoundError return value bug

d316157

buck54321 force-pushed the btcspv branch from 6546f64 to d316157 Compare October 21, 2021 16:23

chappjc reviewed Oct 21, 2021

View reviewed changes

buck54321 force-pushed the btcspv branch from 0520258 to 810d02b Compare October 21, 2021 20:30

rotator shant die

010b451

buck54321 force-pushed the btcspv branch from 810d02b to 010b451 Compare October 21, 2021 20:35

chappjc approved these changes Oct 21, 2021

View reviewed changes

chappjc merged commit d202f6d into decred:master Oct 21, 2021

		w.log.Debugf("A spending input was found during the scan but the output " +
		"itself wasn't found. Was the startBlockHeight early enough?")

		// Probably not worth doing. Just let decodeWallet_v0 append the nil and pass it
		// up the chain.

client: bitcoin spv #1230

client: bitcoin spv #1230

Conversation

buck54321 commented Oct 11, 2021

client/asset:

client/asset/btc:

front-end:

client/db:

buck54321 commented Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

chappjc Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

chappjc commented Oct 13, 2021 • edited Loading

This comment has been minimized.

chappjc commented Oct 13, 2021 • edited Loading

chappjc left a comment

Choose a reason for hiding this comment

chappjc Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Oct 14, 2021 • edited Loading

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Oct 14, 2021 • edited Loading

itswisdomagain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

itswisdomagain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 commented Oct 21, 2021 • edited Loading

buck54321 commented Oct 21, 2021

chappjc Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

buck54321 commented Oct 12, 2021 •

edited

Loading

chappjc Oct 13, 2021 •

edited

Loading

chappjc commented Oct 13, 2021 •

edited

Loading

chappjc commented Oct 13, 2021 •

edited

Loading

chappjc Oct 13, 2021 •

edited

Loading

chappjc commented Oct 14, 2021 •

edited

Loading

chappjc commented Oct 14, 2021 •

edited

Loading

JoeGruffins Oct 18, 2021 •

edited

Loading

buck54321 commented Oct 21, 2021 •

edited

Loading

chappjc Oct 21, 2021 •

edited

Loading

chappjc Oct 21, 2021 •

edited

Loading